UCT/IB/MLX5: Track RC send PSN with path MTU - #11821
Conversation
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The BlueField gtest run aborted on Full analysisSummary: Root cause: Commit Implicated commit: File: src/uct/ib/mlx5/gga/gga_mlx5.c:528-533 (missing init); assert at src/uct/ib/mlx5/rc/rc_mlx5.inl:472; existing init at src/uct/ib/mlx5/rc/rc_mlx5_ep.c:979 Suggested fix: Preferred: move the initialization into the shared connect helper so every transport that owns an uct_rc_mlx5_txwq_set_path_mtu(&ep->super.tx.wq, path_mtu);Additionally consider giving Related: PR #11821 (branch
|
Report the payload length while building mlx5 data segments so RC PSN tracking does not walk the zcopy IOV again on the send fast path.
Place the cached path MTU fields after the send indices so they do not introduce padding before the MMIO register pointer.
Keep the next-PSN counter free-running so RC posting only performs an addition. Mask the value when exposing a hardware PSN through the accessor and VFS.
|
🤖 CI Triage Agent — TL;DR: The ASAN gtest aborted on Full analysisSummary: Root cause: PR #11821 adds RC send-PSN tracking based on path MTU: Implicated commit: ab74f2b — "UCT/IB/MLX5: Track RC send PSN with path MTU", Roie Danino (branch File: src/uct/ib/mlx5/gga/gga_mlx5.c:528-534 (missing init); assertion site src/uct/ib/mlx5/rc/rc_mlx5.inl:472 Suggested fix: In uct_rc_mlx5_txwq_set_path_mtu(&ep->super.tx.wq, path_mtu);mirroring |
|
🤖 CI Triage Agent — TL;DR: The build broke — Full analysisSummary: "Setup Server" stage failed at Root cause: Implicated commit: [REDACTED:Hex High Entropy String] — "UCT/IB/MLX5: Limit PSN tracking to RC packet WQEs", Roie Danino (part of PR #11821 / branch pr11684-rc-psn; builds on ab74f2b, 31d155a, 119b4a4 by the same author) File: src/uct/ib/mlx5/rc/rc_mlx5.inl:489 Suggested fix: Don't reference
#if HAVE_MLX5_MMO
# define UCT_IB_MLX5_OPCODE_MMO MLX5_OPCODE_MMO
#else
/* MMO opcode is unavailable in this rdma-core; value is never posted */
# define UCT_IB_MLX5_OPCODE_MMO 0x2fu
#endifand use
Also worth adding a CI/build check on a host without Related: PR #11821 (this PR), predecessor PR #11684 (branch name
|
4043a27 to
cb95c06
Compare
| static UCS_F_ALWAYS_INLINE void | ||
| uct_rc_mlx5_txwq_set_path_mtu(uct_ib_mlx5_txwq_t *txwq, |
There was a problem hiding this comment.
I would expect this function to be called from control path, maybe move it to .h/.c?
| wqe_size = sizeof(*ctrl) + sizeof(*dma_seg) + (2 * sizeof(*dptr)); | ||
| opmod = UCT_IB_MLX5_OPMOD_MMO_DMA; | ||
| message_length = 0; |
| txwq->hw_ci = 0xFFFF; | ||
| txwq->flags = 0; |
| (message_length + txwq->path_mtu_mask) >> | ||
| txwq->path_mtu_shift); |
There was a problem hiding this comment.
add a variable for this math and ucs_assert(res > 0), I think ucs_max is redundant here sinse NOP is handled separately
| txwq->path_mtu_mask = mtu - 1; | ||
| txwq->path_mtu_shift = ucs_ilog2(mtu); |
There was a problem hiding this comment.
maybe cache only path_mtu_shift and calc UCS_MASK(path_mtu_shift) inplace?
There was a problem hiding this comment.
Thought about it, that will add about 2-3 operations in the fast path:
- res = 1 << mtu_shift
- mask = (res - 1)
and only then the division itself
wdyt?
There was a problem hiding this comment.
I'm not sure that is better - the couple of instructions on register or double load from the same cache line, it may be arch specific but if we can save some space for +/- the same price - why not... I would check with asm + test MSG rate bench
| dptr = (struct mlx5_wqe_data_seg *)((char *)inl + inl_seg_size); | ||
| wqe_size = ctrl_av_size + inl_seg_size + | ||
| uct_ib_mlx5_set_data_seg_iov(txwq, dptr, iov, iovcnt); | ||
| opmod = 0; | ||
| wqe_size = ctrl_av_size + inl_seg_size + | ||
| uct_ib_mlx5_set_data_seg_iov_length( | ||
| txwq, dptr, iov, iovcnt, &iov_length); | ||
| opmod = 0; | ||
| message_length = iov_length + sizeof(*rch) + am_hdr_len; |
| dptr = uct_ib_mlx5_txwq_wrap_exact(txwq, (char *)inl + inl_seg_size); | ||
| wqe_size = ctrl_av_size + inl_seg_size + | ||
| uct_ib_mlx5_set_data_seg_iov(txwq, dptr, iov, iovcnt); | ||
| opmod = 0; | ||
| wqe_size = ctrl_av_size + inl_seg_size + | ||
| uct_ib_mlx5_set_data_seg_iov_length( | ||
| txwq, dptr, iov, iovcnt, &iov_length); | ||
| opmod = 0; | ||
| message_length = iov_length + sizeof(struct ibv_tmh); |
|
🤖 Starting review — findings will be posted here when done. |
| uct_ib_mlx5_qp_t super; | ||
| uint16_t sw_pi; /* PI for next WQE */ | ||
| uint16_t prev_sw_pi; /* PI where last WQE *started* */ | ||
| uint32_t next_first_psn; /* Free-running PSN counter */ |
There was a problem hiding this comment.
The new next_first_psn / path_mtu_mask / path_mtu_shift state and the uct_ib_mlx5_txwq_get_next_first_psn() accessor are only read by the gtest and the new VFS file; there is no production consumer in this PR. This is fine as preparatory infrastructure, but worth confirming a follow-up will consume it so the state does not remain dead (REVIEW.md "avoid redundant state").
There was a problem hiding this comment.
It will be used in followup PRs
|
🤖 CI Triage Agent — TL;DR: The ASAN gtest run aborted on a fatal debug assertion in Full analysisSummary: Root cause: On a DCI completion error, Implicated commit: Not the PR head (9c3c513 "UCT/IB/MLX5: Centralize RC PSN update guard", Roie Danino — PSN path is RC-only). The exposed defect belongs to the UCP FT lane-recovery series that reuses failed lanes: db208ee (#11563) / 9db2e48 (#11562) / 4cb9dbb (#11266), Evgeny Leksikov, combined with DC failure handling in File: src/uct/ib/mlx5/rc/rc_mlx5.inl:511-515 (assert site); real defect at src/uct/ib/mlx5/dc/dc_mlx5_ep.c:1787-1808 and src/uct/ib/mlx5/dc/dc_mlx5.c:1439,1883-1918 Suggested fix: Make the DC transport guarantee that no non-NOP WQE can be posted on a failed DCI: in Related: #11823 (UCT/IB/MLX5: rc support in progress error handler), #11563, #11397 (previous disabling of a failing fault-tolerance test), PR under test: #11821 |
Signed-off-by: Roie Danino <rdanino@nvidia.com>
Signed-off-by: Roie Danino <rdanino@nvidia.com>
Signed-off-by: Roie Danino <rdanino@nvidia.com>
Signed-off-by: Roie Danino <rdanino@nvidia.com>
Signed-off-by: Roie Danino <rdanino@nvidia.com>
Signed-off-by: Roie Danino <rdanino@nvidia.com>
Signed-off-by: Roie Danino <rdanino@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
| uct_ib_mlx5_txwq_t *txwq, | ||
| uct_ib_mlx5_qp_attr_t *attr); | ||
|
|
||
| void uct_rc_mlx5_txwq_set_path_mtu(uct_ib_mlx5_txwq_t *txwq, |
There was a problem hiding this comment.
minor — misaligned continuation line. The enum ibv_mtu path_mtu line has one extra leading space; suggested:
| void uct_rc_mlx5_txwq_set_path_mtu(uct_ib_mlx5_txwq_t *txwq, | |
| void uct_rc_mlx5_txwq_set_path_mtu(uct_ib_mlx5_txwq_t *txwq, | |
| enum ibv_mtu path_mtu); |
The same misalignment exists in the definition in rc_mlx5_ep.c.
| valid_length, buffer, max); | ||
| } | ||
|
|
||
| void uct_rc_mlx5_txwq_set_path_mtu(uct_ib_mlx5_txwq_t *txwq, |
There was a problem hiding this comment.
minor — misaligned continuation line. The enum ibv_mtu path_mtu line has one extra leading space, matching the misalignment in the declaration in rc_mlx5.h.
| uct_ib_mlx5_set_ctrl_qpn_ds(uct_ib_mlx5_txwq_get_wqe(txwq, 0xffff), 0, 1); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
minor — double blank lines. The new uct_ib_mlx5_txwq_vfs_show_next_first_psn has two blank lines before and after it, while the surrounding functions in this file are separated by a single blank line. Please use a single blank line for consistency.
| strb, "%u\n", uct_ib_mlx5_txwq_get_next_first_psn(txwq)); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
minor — double blank lines after uct_ib_mlx5_txwq_vfs_show_next_first_psn. Please use a single blank line for consistency.
|
Observation (non-blocking): Observation (non-blocking): DC ( Observation (non-blocking): The Test coverage: The new |
Signed-off-by: Roie Danino <rdanino@nvidia.com>
Signed-off-by: Roie Danino <rdanino@nvidia.com>
Signed-off-by: Roie Danino <rdanino@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
| { | ||
| ucs_assert(txwq->path_mtu_shift > 0); | ||
|
|
||
| return ucs_max(1u, (message_length + txwq->path_mtu_mask) >> |
There was a problem hiding this comment.
why do we need ucx_max here?
There was a problem hiding this comment.
Zero-length PUT_ZCOPY is valid (UCT_CHECK_LENGTH(..., 0, ...)) and still posts an RC WQE that consumes one PSN. Without ucs_max, the tracked PSN would not advance; NOP is handled separately.
There was a problem hiding this comment.
so maybe handle 0-lenght PUT_ZCOPY separately as well?
There was a problem hiding this comment.
Zero length is not specific to PUT_ZCOPY. PUT_BCOPY and individual PUT_SGL_ZCOPY entries can also post zero-length RDMA WRITE WQEs, each consuming one PSN, while PUT_SHORT already counts one. Keeping ucs_max(1u, ...) in the common helper handles all posted RC WQEs consistently. NOP and MMO bypass this helper because they do not consume an RC wire PSN.
| uct_ib_mlx5_qp_t super; | ||
| uint16_t sw_pi; /* PI for next WQE */ | ||
| uint16_t prev_sw_pi; /* PI where last WQE *started* */ | ||
| uint32_t next_first_psn; /* Free-running PSN counter */ |
There was a problem hiding this comment.
| uint32_t next_first_psn; /* Free-running PSN counter */ | |
| uint32_t next_wqe_psn; /* 1st PSN of the next WQE to be posted */ |
Signed-off-by: Roie Danino <rdanino@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
| @@ -212,6 +212,7 @@ uct_rc_mlx5_base_ep_put_sgl_zcopy(uct_ep_h tl_ep, void * const *buffers, | |||
| uct_ib_mlx5_txwq_t *txwq = &ep->tx.wq; | |||
There was a problem hiding this comment.
The DM short paths (put_short/am_short/am_short_iov via uct_rc_mlx5_common_ep_short_dm, and tag_eager_short) post an RC WQE but do not call uct_rc_mlx5_txwq_add_psn, so next_wqe_psn drifts when device memory is used (HAVE_IBV_DM and payload fits in DM segment). Unlike their inline counterparts (uct_rc_mlx5_txqp_inline_post, uct_rc_mlx5_txqp_inline_iov_post), the PSN is not advanced. This is non-blocking since the counter currently has no functional consumer beyond VFS/gtest, but if the intent is accurate PSN tracking, the DM path should also bump the PSN or be explicitly documented as excluded.
There was a problem hiding this comment.
The DM path is already covered indirectly through uct_rc_mlx5_common_txqp_bcopy_post() -> uct_rc_mlx5_txqp_dptr_post(), which updates PSN using hdr_len + iov_length. RC passes IBV_QPT_RC, while the DC update is compiled out, so an additional bump would double-count.
| @@ -144,6 +144,90 @@ UCS_TEST_P(test_rc, fence_am_short_consumed, "RC_FENCE=weak") | |||
|
|
|||
There was a problem hiding this comment.
Residual coverage gap: the new test_rc_mlx5_psn only exercises inline am_short and put_zcopy; it does not cover the DM short path or the put_sgl_zcopy multi-WQE PSN accumulation, so the DM PSN drift would not be caught by CI.
| static UCS_F_ALWAYS_INLINE | ||
| size_t uct_ib_mlx5_set_data_seg_iov(uct_ib_mlx5_txwq_t *txwq, | ||
| struct mlx5_wqe_data_seg *dptr, | ||
| const uct_iov_t *iov, size_t iovcnt) | ||
| { | ||
| return uct_ib_mlx5_set_data_seg_iov_length(txwq, dptr, iov, iovcnt, NULL); | ||
| } |
There was a problem hiding this comment.
minor: it looks redundant
Signed-off-by: Roie Danino <rdanino@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
|
The new tests run only under |
| } uct_ib_mlx5_txwq_t; | ||
|
|
||
|
|
||
| static UCS_F_ALWAYS_INLINE uint32_t |
There was a problem hiding this comment.
Minor: extract a small helper to centralize the 24-bit PSN masking, so future changes related to PSN would reuse this and make it easier to maintain.
Something like:
static UCS_F_ALWAYS_INLINE uint32_t
uct_ib_mlx5_psn24(uint32_t psn)
{
return psn & UCS_MASK(24);
}We might need this in later PRs.
There was a problem hiding this comment.
I would prefer to keep it inline for now and introduce a shared helper in the follow-up PR when it has multiple users.
| size_t uct_ib_mlx5_set_data_seg_iov(uct_ib_mlx5_txwq_t *txwq, | ||
| struct mlx5_wqe_data_seg *dptr, | ||
| const uct_iov_t *iov, size_t iovcnt) | ||
| size_t uct_ib_mlx5_set_data_seg_iov_length(uct_ib_mlx5_txwq_t *txwq, |
There was a problem hiding this comment.
Maybe update the existing uct_ib_mlx5_set_data_seg_iov signature and its callers instead of introducing a new uct_ib_mlx5_set_data_seg_iov_length function, since length is confusing and looks like an input.
Think about adding iov_length to the last argument and updating the 4 call sites in this PR to use the new parameter name.
WDYT?
Signed-off-by: Roie Danino <rdanino@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
Signed-off-by: Roie Danino <rdanino@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
Signed-off-by: Roie Danino <rdanino@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
| #endif | ||
|
|
||
| uct_rc_mlx5_txwq_update_psn(txwq, qp_type, message_length); | ||
| if (opcode_flags == MLX5_OPCODE_RDMA_WRITE) { |
There was a problem hiding this comment.
Zero-length RDMA_READ with message_length==0 would get update_psn(0)=0 PSN added because the special case only checks RDMA_WRITE, not RDMA_READ. This doesn't affect real paths (get_zcopy has a min length check and zero-length reads via dptr_post_iov don't occur), but it is a slight inconsistency with the dptr_post (bcopy) path which adds 1 for both READ and WRITE on the zero-length case.
| @@ -686,13 +686,17 @@ typedef struct uct_ib_mlx5_txwq { | |||
| uct_ib_mlx5_qp_t super; | |||
There was a problem hiding this comment.
Non-blocking (previously noted): next_wqe_psn is stored unmasked as uint32_t while the accessor masks to 24 bits.
| @@ -144,6 +144,108 @@ UCS_TEST_P(test_rc, fence_am_short_consumed, "RC_FENCE=weak") | |||
|
|
|||
There was a problem hiding this comment.
Residual coverage gap (already noted in prior rounds): the new test_rc_mlx5_psn tests require HAVE_MLX5_DV, PUT_ZCOPY support, and mismatched path MTUs between the two entities, so they will be skipped on many CI configurations.
|
Non-blocking (previously noted): next_wqe_psn currently has no consumer beyond VFS/gtest, which appears to be intentional follow-up infrastructure. |
5 participants
What?
Track the PSN that will be used by the next RC mlx5 send WQE.
Cache the path MTU programmed on the connected RC QP and use it to advance the tracked PSN for RC send operations.
Why?
Follow-up RC recovery logic needs a stable mapping from posted WQEs to their first packet sequence number. Using the interface MTU can miscount packets when the connected QP negotiated a different path MTU.
This change is split from #11684 and is intentionally limited to RC. DC, GGA, and recovery classification state are outside its scope.
How?
Cache path_mtu - 1 as a 16-bit mask and log2(path_mtu) as an 8-bit shift when the RC data QP is connected. Packet counts use (length + mask) >> shift, avoiding division and an out-of-line MTU lookup on the send path.
Advance next_first_psn after successful inline, data-pointer, IOV, tag, and SGL posts, wrapping in the 24-bit RC PSN space.